refactor(digest): one owner builds the stored SHA-256 envelope - #5337
sakurahello1 wants to merge 6 commits into
Conversation
Refs loopx-project#5336. The shape owner recognizes `sha256:<64 lowercase hex>` and exports nothing else: the leaf is generated, and the generator deliberately refuses syntax it cannot translate, so writing the envelope had no counterpart. 94 sites in loopx/ concatenated the prefix by hand, 10 of them in periodic_report, each restating the envelope that the same package's readers then checked against the owner's pattern. The new module borrows the owner's pattern object rather than restating a shape and refuses any value the owner would not recognize, so a producer cannot emit a digest its reader rejects. Migrated sites keep their own byte recipes -- two canonicalize with ensure_ascii=True and one hashes a string rather than canonical JSON -- because the envelope is the shared decision and the bytes are each surface's own question. No digest value changes. The three remaining periodic_report sites are census hosts whose import lines are pinned by row number in the project-registry I/O manifest; they are recorded as deferred rather than converted blind. Signed-off-by: sakurahello1 <201035361+sakurahello1@users.noreply.github.com>
A second architecture test covers what the existing single-owner guard names as its own unfinished half: hand-built envelopes. It judges folded values, so a prefix moved into a module constant is the same offender as a literal, and it records the three not-yet-converted files in an allowlist that fails if one of them converts itself, so the list cannot rot. The behavioural cases enter through each migrated helper and compare against the expression it replaced, which is the invariant that makes the conversion safe to land. The new module is pinned into the existing guard's consumer manifest. Signed-off-by: sakurahello1 <201035361+sakurahello1@users.noreply.github.com>
The semantic inventory groups named string constants across modules, and `ENVELOPE_PREFIX` was already bound in handoff_fragments.py to a different value. Naming it after what it envelopes makes the collision disappear: the drift smoke returns to its reviewed budgets instead of growing three of them (conflicting_values, conflicting_definitions and the semantic half of the first), which is the check this repository uses to stop a consolidation from quietly adding a second meaning to a name. Signed-off-by: sakurahello1 <201035361+sakurahello1@users.noreply.github.com>
…ound Written after running the mutations, each of which found a real gap: * folding only module-level assignments let a converted file move the prefix into a function-local name and keep building the envelope unnoticed; the folder now walks every scope and the bypass corpus carries that form; * `re.compile` caches by pattern and flags, so a restated copy of the owner's pattern compares *identical* to the borrowed object -- the identity assertion proves nothing on its own, so the builder is now also required to compile no pattern of its own; * the event-digest recipe used ASCII-only input, where `ensure_ascii` is not observable and flipping it passed. A non-ASCII id is the only input that pins the byte recipe this PR deliberately left per-module. Signed-off-by: sakurahello1 <201035361+sakurahello1@users.noreply.github.com>
cocolord
left a comment
cocolord
left a comment
There was a problem hiding this comment.
评审提交：cc93a8932e123211e9f5027e80f970eb57210e3c。这是 policy-11 whole-PR、exact-head 评审。我检查了 12 个变更文件、#5336 的自述目标、所有 periodic-report 迁移点、新 builder、两层 architecture tests、远端 checks，并在真实测试 helper 上做了反例验证。生产替换保持 digest 值等价，但新增 guard 不能可靠证明它声称的 single-owner 约束，而且这一“首个 surface”仍留下同包内 3 个手写 producer；当前成本与收益不匹配。
动机
把 sha256:<hex> 的构造集中到一个 helper，方向上能减少 writer/reader 规则漂移；periodic-report 的多个私有 digest helper 确实重复了相同 envelope 拼接。若一个完整 capability 都迁到稳定 owner，后续修改格式、验证非法输入和审查调用方会更集中。
但这是无可观察行为变化的维护性 refactor，收益门槛应高于“发现很多相同字符串”。当前 PR 用 455 行新增、28 行删除替换 10 个一行构造，其中 381 行是新的 bespoke AST guard；同时同一 periodic-report package 仍保留 3 个手写 producer，仓库其他 97 个 Python/TypeScript site 也不受此 guard 约束。作者需要把可持续收益做成真实、完整且低维护成本的边界，而不是把第一批机械替换本身当成收益。
改动思路
digest_envelope.py 新增 enveloped_sha256 与 sha256_envelope：前者拼接 prefix 后复用既有 ENVELOPED_SHA256_PATTERN 做 fullmatch，后者计算 SHA-256 再调用前者。9 个 periodic-report 模块的 10 个构造点改为调用该 helper，各模块原有 JSON canonicalization、字符串编码与截断语义保持在原 owner。
新增 test_content_digest_production_owner.py 一方面逐个调用迁移后的 helper 与旧表达式比较，另一方面扫描所谓“converted surface”，阻止再次手写 prefix；3 个未迁移文件通过 CONVERTED_SURFACES allowlist 排除。既有 test_content_digest_single_owner.py 只新增新 builder 的 consumer 声明。正向值等价路径成立，负向 guard 路径却有结构性缺口。
具体改动
阻塞问题
-
[P1] 新 production scan 的 name folding 不遵守 Python lexical scope，既误报也漏报。
_string_constants用一次全树ast.walk把所有作用域的同名变量压进一个 flat dict，再由_denotes_prefix在任意位置查询。reviewer 直接调用 exact-head 的_hand_built_envelopes复现：一个函数里未使用的 localprefix = "sha256:"会让另一个函数的参数prefix + value被误报；而 annotated constant、str.format和str.join三种手写 envelope 都返回空命中。这个 scanner 因此既会阻塞无关后续代码，又可以被常见写法绕过，不能支撑“converted surface 只能经 owner 构造”的核心收益。更关键的是，同仓库现有 single-owner guard 已有 scope-aware_Scope/_collect_scopes/_fold_text，并专门测试 local binding 不泄漏到其他 scope；新文件复制了一套更弱实现。请抽取/复用现有 folding owner，至少覆盖 AnnAssign 与明确支持的构造，并把无法判定的 form 报为 unknown，而不是当成无违规。 -
[P1] 请完成一个真实 periodic-report 边界，或把这 381 行 guard 的独立收益讲清并显著收窄。
CONVERTED_SURFACES名义上登记整个loopx/capabilities/periodic_report，却主动跳过pending_intent.py、post_writeback_hook.py、request_action.py的手写 producer；理由只是新增 import 会移动已生成 manifest 的行号。生成并审核 manifest 本来就是这次同域机械迁移的一部分，不构成独立架构边界。结果是一个 capability 内同时保留新旧 producer，新增 381 行测试和 deferred protocol，却没有交付“一个 surface 只有一个 builder”的完整收益。请优先迁移这 3 个同包 site 并更新 manifest，然后把 guard 缩到真正稳定的 contract；若仍要保留分阶段例外，请明确给出受影响用户/维护者、before/after 可观察收益、预计消除的实际失败或维护成本，以及为什么这些收益足以抵消新增 AST framework 的长期成本。
关键代码讲解
enveloped_sha256复用 reader 的ENVELOPED_SHA256_PATTERN验证构造结果，sha256_envelope只负责 bytes -> lowercase digest -> envelope；这两个函数本身边界清晰。- periodic-report 的
_digest/_canonical_digest/_content_digest保留各自 canonical bytes，只替换最后的 prefix 拼接。focused parity 覆盖了 non-ASCIIensure_ascii=True分支，未发现值变化。 _string_constants、_denotes_prefix、_hand_built_envelopes是新 guard 的实际 decision owner；由于 flat name table 和有限 AST forms，其通过不等于 single-owner invariant 成立。CONVERTED_SURFACES同时充当扫描范围和临时例外清单，但三个例外与迁移点处在同一 package、同一 change reason，没有真实部署或兼容边界。- existing
test_content_digest_single_owner.py已实现 scope-aware binding/folding；新增 381 行测试没有复用这个更强 owner，是重复知识而不是必要隔离。
对主干的风险
生产路径的直接行为风险较低：两组 focused tests 分别通过 242 和 268 个用例，迁移 helper 的输出与旧表达式相同，非法 bare digest 也被拒绝。远端仅 test-shard 1/2、aggregate pytest 与 merge-gate 为红；我在 immutable base 和 exact head 上重跑三个具体失败，双方都是相同的 turn-contract count 与 prompt-upgrade-hook 断言，因此这些 CI 红灯是既有基线问题，不归因于本 PR，也不是本次拒绝理由。
真正风险在长期维护：这个 PR 的主要净新增是一个不健全的静态 analyzer。它会制造 false-positive CI 阻塞，也会让常见 bypass 在绿色测试下继续手写；同时临时 allowlist 把同一 capability 的剩余迁移固化为永久维护面。失败后没有 runtime 回退，维护者只能调 scanner、扩大例外或继续追加 bypass case，正是本次 refactor 想消除的重复成本。
语义与 CI 对齐
digest 的 runtime 值语义在已迁移调用点保持一致，没有默认用户行为变化，也没有新权限、状态机或 guidance/obligation 混淆。当前不对齐的是 architecture claim：测试名和 issue 使用“one owner / converted surface”，但 scanner 无法证明这一点，且该 surface 仍显式包含三条 legacy producer。应先让 contract 名称、扫描能力和实际迁移范围一致，再把它作为长期 required guard。
我的整体评价
结论是 REQUEST_CHANGES。小型 builder 与机械替换本身可以接受，且作者对 byte recipe 差异的测试是认真且正向的；但 whole PR 当前为 10 个一行替换引入 381 行重复且不健全的 AST enforcement，并用 manifest 行号作为同包迁移的暂停理由。对长期工程质量，这是净复杂度上升而非已经证明的收敛。
建议把 scope-aware folding 从现有 single-owner guard 抽成一个复用 helper，完成 periodic-report 同包 3 个剩余 producer 和 manifest 更新，再保留薄的行为 parity/architecture assertion。若不同意这条更小路径，请在 PR 中明确量化实际维护收益与失败历史，而不只是 site 数量和假设性 drift。修复后需要加入上述 false-positive/false-negative scanner 反例、重跑 focused suites 与 required CI。本次 review 不授权 merge。
English verdict: REQUEST_CHANGES on exact head cc93a8932e123211e9f5027e80f970eb57210e3c. The migrated digest values are behaviorally equivalent and the focused suites pass, but the new 381-line production guard is not a sound ownership check: its flat name table leaks constants across lexical scopes, while annotated constants, .format(), and .join() bypass it. It also duplicates the repository's existing scope-aware folding machinery. The claimed converted periodic-report surface still exempts three same-package producers solely to avoid regenerating a manifest, leaving the refactor fragmented and its maintenance ROI unproven. Reuse/extract the existing scope model, finish or honestly narrow the surface, provide concrete before/after maintenance benefit, and rerun the targeted and required checks.
Signed-off-by: 牛锐博 <niuruibo@niuruibodeMacBook-Air.local>
sakurahello1
left a comment
sakurahello1
left a comment
There was a problem hiding this comment.
Reviewed exact head 8e5bd950b74f73290b82520f46aca5e0446b2b51.
No new actionable findings. The prior change-request review applied to cc93a8932; this head replaces the flat AST binding table with the existing scope-aware folding helpers, adds negative coverage for annotated local binding plus .format() and .join(), and completes the three remaining periodic_report producer migrations with the manifest row updates.
Validated locally: uv run --extra test pytest tests/architecture/test_content_digest_production_owner.py tests/architecture/test_content_digest_single_owner.py tests/capabilities/test_periodic_report_adapters.py tests/capabilities/test_periodic_report_audience.py tests/capabilities/test_periodic_report_bindings.py tests/capabilities/test_periodic_report_cadence_journal.py tests/capabilities/test_periodic_report_runtime_producer.py tests/capabilities/test_periodic_report_triggers.py tests/capabilities/test_periodic_report_workspace.py — 341 passed. Remote required checks are green at this head.
huangruiteng
left a comment
huangruiteng
left a comment
There was a problem hiding this comment.
[P2] Folded f-string prefix still bypasses the new producer-ownership guard
Reviewed exact head: 87dd13ad9efa14df16e6b541fe0a0c726f6959ba; immutable merge base: 3b73108e32acfe6657204b902a037171797e3e5c. 当前 LoopX pull_request_review_execution_contract_v2（policy revision12）完整评审。结论 REQUEST_CHANGES，仅因下面已复现的 guard 缺口；不是远端红CI或作者身份。
动机
#5336 的真实目标是补齐“reader共享shape、writer却手写envelope”的缺口，并完成一个可维护的periodic_report目录切片。评审范围以当前16文件和已纠正方向为准，不继承旧PR说明中9个non-census站点及3个排除项。共享writer应减少重复知识，不能以绿色但可绕过的owner assertion代替有效维护边界。
改动思路
这次正确复用了既有TS生成的 ENVELOPED_SHA256_PATTERN 对象，并把Python字节hash/封装放在46行手写sibling，未扩大只输出两个pattern的生成leaf，也未新建Python状态决策源。每个caller自己的JSON/string/ASCII配方保留；共享的只是envelope。之前flat binding table跨作用域泄漏已改为复用 _collect_scopes/_fold_text/_scope_of，剩余3个同包producer及manifest也已补齐。这些旧问题确实解决；本轮不是要求重做整个refactor。
具体改动
完整diff包含12个periodic_report producer模块、shared builder、两处census line anchor与architecture guard。archive/adapters的UTF8内容、通常canonicalJSON、runtime event的sorted＋ensure_ascii=True、manual/stage JSON默认ASCII，以及cadence已知bare hex的包装均保持原配方。迁移后的request/pending/post-writeback路径没有再以manifest行号为由暂停。260-site census check通过。
关键代码讲解
digest_envelope.py:30 enveloped_sha256：加一次prefix，再借reader同一pattern fullmatch；拒绝短/大写/已带envelope等无效值，不重述正则。sha256_envelope只hash已有bytes。runtime_producer.py:43 _event_digest：排序event IDs并保留ensure_ascii=True和compactJSON，再调用shared builder；因此非ASCIIevent的public trigger/report身份不应变化。post_writeback_hook.py:390 evaluate_periodic_report_trigger_evaluation_intent：复用既有意图/eligibility流程，cadence用已知bare hex包装而不是再次hash，manual/stage继续旧bytes。test_content_digest_production_owner.py:64 _hand_built_envelopes：Add、%格式、format和join使用既有scope fold；但71–77行的JoinedStr分支只看literal Constant，跳过FormattedValue里可静态折叠的prefix。whole-tree另一条检查又只看module-level ast.Assign，函数内prefix和module AnnAssign都不会兜底。
需要修复的具体问题
函数内 prefix = "sha256:"，随后 return f"{prefix}{hashlib.sha256(encoded).hexdigest()}"，属于普通、静态已知的手写envelope，不是任意动态Python。当前 _hand_built_envelopes 返回空列表；同一local prefix用 prefix + value 则被正确捕获。在临时fixture里，我仅将当前完整 runtime_producer._event_digest 的builder调用恢复成上述旧式手写，随后实际执行 test_converted_surfaces_build_the_envelope_only_through_the_owner 和 test_the_owner_is_the_only_module_that_states_the_prefix_rule：两条都通过；独立“应该拒绝手写producer”的断言失败。原源码和active Goal没有被修改。
最小修复是让JoinedStr对已知前导FormattedValue复用现有scope-aware _fold_text。owner排除应按文件位置进行，而不是让owner使用的拼写在所有文件都合法。增加local Assign/AnnAssign、module AnnAssign f-string反例和shadowing negative，然后重跑现有两套architecture与report/native checks；不需要引入新AST解释器、宽迁移或改变runtime身份。
对主干的风险
当前迁移运行时没有发现错误：我自己跑了515项architecture＋全部periodic_report测试，全15个changedPython Ruff、19-source Mypy、changed-diff advisory、260-site census、完整semantic smoke；初始16项premerge也通过。immutable base/head同一harness实际执行public compose-run、evaluate-runtime-trigger（非ASCIIevent）、unsafe-event拒绝，及File cadence跨later restart/peer的pending coalescing、intent续跑、publication cursor commit/retry/独立readback与covered-fact抑制，13个完整观察完全一致，8个helper旧公式也一致。只归一化临时fixture-root，未删除ID、失败detail或effects。
另做actual CLI故障注入：把runtime producer bound builder改为bareSHA256，CLI仍eligible/promoted，却改变trigger identity。只看成功的第一版review oracle会漏掉；补上独立冻结legacy身份后正确失败，正常base/head通过。原uninformative和失败回执均保留，不能把“返回成功”当语义parity。这与上面的静态guard false-negative是两个不同验证结论：前者证明runtime identity敏感性，后者是本PR仍需修的P2。
语义与CI对齐
没有新state/authority/actor协议、CLI参数、默认开关或机器work obligation。Shared error用generic content digest，不把report词写入通用worklane。digest shape不证明内容真实性或权限。完整diff不改frontend、Lark sink、settings或普通操作步骤，因此没有需要新增的companion；外部report delivery、installedfrontend和PG authority未触及也未宣称验证。wait_for_ci=false：未获取、轮询或等待远端CI，515测试与canary绿不抵消独立ownership反例。CQA初始record/verify及premerge成功回执保留，发现后必须写入未解决blocker并持有merge readiness，而不是继续使用all-clear记录。
我的整体评价
REQUEST_CHANGES，范围仅这一条P2：运行时配方迁移、完整目录收敛、旧scope-leak修复和census同步均有正向证据；剩下的是新增guard漏掉普通folded f-string，而不是要求静态分析覆盖任意Python。future-facing pass已落实共享builder与现有scope model复用；现在只需在该相邻owner补一处分支及薄反例，恢复承诺的检查能力。全仓其他producer的迁移不是本PR验收，也不用为已完成部分增加仪式性Todo。本轮不授权merge、不做自合并。
English verdict: REQUEST_CHANGES - exact head 87dd13a. Runtime digests, public trigger identities and durable cadence/publication behavior are preserved, and prior package-completion/scope-reuse issues are resolved. One concrete P2 remains: a statically known local sha256: prefix used in an f-string bypasses both actual ownership guards. Reuse existing scoped folding for leading FormattedValue parts and add narrow regression cases; unrelated CI is not the rejection reason.
Goal And Delivered Outcome
Outcome basis / optional anchor: Closes [Architecture]: the digest owner recognizes an envelope, 94 sites still build it by hand #5336, which is anchored in the repository's own text:
tests/architecture/test_content_digest_single_owner.pyends its docstring with "producers that concatenate\"sha256:\"by hand are the other half of the decision and are deliberately unchanged". The single-owner guard therefore already names this gap and already declines to close it silently.Goal/source and gap: the shape owner recognizes the two whole-value digest shapes and exports nothing else. It is generated (
scripts/generate_semantic_bindings.py), and that generator refuses every construct except two flagless literal patterns -- by design, "Reject extra syntax rather than execute TS or guess its meaning" -- andtest_the_owner_is_a_leaf_and_exports_only_the_two_shapesasserts the leaf's namespace is exactly{re, BARE_SHA256_PATTERN, ENVELOPED_SHA256_PATTERN}. So there was no production counterpart, and every writer restated the envelope instead of asking for it.Measured on the base revision: 94 hand-built sites in 81 Python files, plus 26 sites in 17 TypeScript files. About thirty of them are near-identical private
_digest/_sha256/_canonical_digesthelpers. Consequence today: a digest written by one module and read by another is validated by the owner's pattern at the reader and by nothing at the writer.Observable before → after, with the validation row that proves it: inside
loopx/capabilities/periodic_report, the envelope is now built in one place (10 sites across 9 files) and an architecture test fails if any converted file restates it. No digest value changes: the behavioural cases call each migrated helper and compare against the expression it replaced.Intended base:
3ec049e13. Closes [Architecture]: the digest owner recognizes an envelope, 94 sites still build it by hand #5336 (the issue I filed for this half; it is not a pre-existing maintainer task).Scope And Continuation
loopx/control_plane/digest_envelope.py), the first surface converted, the production scan added, and the new module pinned into the existing guard'sCONSUMER_MODULES(one line -- layer 2 of that guard requires every importer of the owner to be listed).pending_intent.py,post_writeback_hook.pyandrequest_action.pyare census hosts whose import lines are pinned by row number inloopx/semantics/project_registry_io_manifest_v1.json. I regenerated that manifest in place to check this branch moves nothing (output identical: 260 sites, 0 unclassified). The remaining 71 Python sites and 26 TypeScript ones stay outside the scan until each surface is converted.CONVERTED_SURFACES; the scan, the allowlist-rot check and the value-based bypass corpus are already generic.Validation
cc93a8932(4 commits, 12 files, +455 -28).unitpytest tests/architecture/test_content_digest_production_owner.py-> 29 passed: the scan over the converted surface, the allowlist-rot check, eight bypass-corpus cases and the builder's own contract.unitpytest tests/architecture/test_content_digest_single_owner.py-> 211 passed with the new module added toCONSUMER_MODULES, so the existing four layers still hold against a second module in the family.unitpytest tests/capabilities/test_periodic_report*.py-> 268 passed;pytest tests/control_plane tests/architecture tests/canary -k "digest or envelope"-> 110 passed.integrationpytest tests/architecture tests/canaryin full, since this branch adds a module and touches a generated-adjacent manifest;tests/architecture tests/canary-> 1101 passed, 1 failed in 1m50s. The one failure,test_new_independent_twin_cannot_hide_behind_generated_pair, is not from this branch: the same six node ids that failed in the first run of this selection were replayed on an unmodified3ec049e13worktree under identical conditions (same venv, same Node on PATH,node_moduleslinked in both trees) and that one id is the only one that fails there too. The other five were caused by this branch and are fixed -- see the inventory row below.regression_parity_reference()in the test, the literal spelling each site used), covering both canonicalization families and the one site that hashes a string rather than canonical JSON. The project-registry I/O manifest regenerated in place is byte-identical, which is what proves no census anchor row moved.staticpython -m ruff checkon every changed path: clean.python -m mypy(no arguments, as CI runs it):Success: no issues found in 19 source files.git diff --check: clean.loopx check --scan-pathon the new module, the migrated package and the new test: public boundary scan clean.staticruff format --checkis not a gate here, and four of the nine migrated files (adapters,cadence_journal,machine_defaults,runtime_producer) already report would-reformat hunks on the unmodified base. I counted hunks per file on base and on head: 5, 10, 2 and 6 in both, so this branch adds none. Both new files are format-clean.mutationre.compilecaches by pattern and flags, so a restated copy of the owner's pattern compares identical to the borrowed object and the identity assertion proved nothing on its own (N7); and the event-digest recipe used ASCII-only input, whereensure_asciiis not observable, so flipping it passed (N8). Remaining caught: a converted file reverted to hand-building (N1, N4); the constant folder removed (N2); the percent arm removed (N3); builder validation removed (N5, 7 cases) and builder emitting the bare digest (N6, 12 cases); the consumer pin removed (N9); the prefix constant renamed back into a name collision (N11, 18 cases).staticENVELOPE_PREFIX, whichhandoff_fragments.pyalready binds to a different value, andexamples/semantic-vocabulary-drift-smoke.pycounts named string constants across modules: the branch grewconflicting_values16->17,conflicting_definitions55->57 andconflicting_values_semantic0->1 -- three budgets, none reviewed. Renaming the constant returns the smoke tookwith every budget at its reviewed value, so this consolidation does not add a second meaning to a name.test-shard (1)andtest-shard (2)are red, with two failing tests between them.test_turn_contract_generation.py::test_new_independent_twin_cannot_hide_behind_generated_pairis the same failure reproduced locally on an unmodified base worktree (above).test_prompt_upgrade_hook.py::test_live_decision_adds_only_existing_required_read_channelI could not see locally at first because CI shards it differently, so I ran that one test file in both trees under identical conditions: 2 failed on this head, the same 2 failed on the unmodified base, same parametrizations. Neither is upstream-of-me in the sense of a main-side aggregate gate -- both are pre-existing repository failures this branch inherits.kernel-static-checks,typescript-core (1/3)and(2/3),chat-bundle,dashboard-acceptanceandwindows-powershellare green at this head.test_the_owner_is_the_only_module_that_states_the_prefix_ruledoes run tree-wide, but only for the narrow fact that no second module binds the prefix as a constant; it cannot see an inline literal, which is what the per-surface scan is for. The byte recipes stay per-module (two useensure_ascii=True, one hashes a string): consolidating those is a different decision about canonicalization, not about the envelope, and this PR does not claim it.See validation disclosure guidance.
Frontend / Visual Evidence
Type of Change
No observable behavior changes: every migrated site emits the same bytes it emitted before, which is what the
regression_parityrow exists to show rather than assert in prose.LoopX Area
Technical Direction
Shared-authority RFC fixture impact
N/A -- no RFC dimension is claimed here. The shared coordination fixture, the envelope and the provider arms are untouched.
Boundary Checklist
none.